libunwind: Use new unwinding APIs under c18n #740

dpgao · 2024-06-20T10:38:34Z

Assembly stubs for _rtld_unw_{get,set}context are removed. Instead, turn calls to these functions into no-ops when they are not defined by RTLD.

dstolfa · 2024-06-20T14:22:37Z

This works fine on my end, but wait for @jrtc27 to review as well.

jrtc27 · 2024-06-20T17:08:54Z

By using the same name for these unversioned functions, you have a compatibility problem when upgrading from 23.05 to a newer version, as there will be a window within which rtld has been upgraded but not libunwind. This isn't limited to when c18n is enabled (which would make it a non-issue), because part of the restoration process is done by the callee currently even for non-c18n, and so with a new rtld you will no longer have those registers restored which, importantly, includes CSP. C2 and C3 are probably unused enough in practice that you wouldn't notice those during that window.

jrtc27 · 2024-06-20T17:11:14Z

Now, having said all that, I had commented before that these functions probably shouldn't be called _rtld_foo, because that's a very FreeBSD-oriented naming scheme. So I think the right approach is to use new, less FreeBSD-y names for them, and the problem will then be avoided. If you enable c18n then things will break unless rtld provides the old names for compatibility, but I think it's fine to just say "don't turn it on when upgrading".

dpgao · 2024-06-20T17:26:04Z

How about __unw_c18n_setcontext and __unw_c18n_getcontext?

jrtc27 · 2024-06-20T17:36:54Z

I would rather see it be a generic API that both {set,long}jmp and unw{get, set}context use, because at the end of the day they want exactly the same thing.

dpgao · 2024-06-21T09:41:17Z

I would rather see it be a generic API that both {set,long}jmp and unw{get, set}context use, because at the end of the day they want exactly the same thing.

RTLD's unwinding code is indeed shared by both *jmp and *context. Separate APIs are provided just because different sealers are used. (See rtld_c18n.c and excerpt below)

_rtld_longjmp(uintptr_t ret, void *rcsp, void *csp)
{
	return (unwind_stack(ret, rcsp, cheri_unseal(csp, sealer_jmpbuf)));
}

uintptr_t
_rtld_unw_setcontext(uintptr_t ret, void *rcsp, void *csp)
{
	if (C18N_ENABLED)
		ret = unwind_stack(ret, rcsp, cheri_unseal(csp, sealer_unwbuf));
	return (ret);
}

dstolfa · 2024-06-21T11:29:25Z

If we do the register restoring part of libunwind inside rtld, we don't need the sealer to be passed to libunwind and thus could probably use the same sealer. This would allow us to use the same APIs sans the if (C18N_ENABLED) check. I'm not sure how setjmp/longjmp work now, but I assume it wouldn't do any harm to add that check to them as well?

dpgao · 2024-06-21T11:38:51Z

If we do the register restoring part of libunwind inside rtld, we don't need the sealer to be passed to libunwind and thus could probably use the same sealer. This would allow us to use the same APIs sans the if (C18N_ENABLED) check.

Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.

I'm not sure how setjmp/longjmp work now, but I assume it wouldn't do any harm to add that check to them as well?

That's true.

jrtc27 · 2024-06-21T11:46:46Z

Restoring of registers from the trusted stack (well, really, extraction into some generic “c18n state” struct for the caller to use, libunwind doesn’t want them actually restored whilst unwinding, just recorded in its context).

dstolfa · 2024-06-21T11:47:34Z

Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.

Why would we have to teach it about libunwind internals? The idea would be to pass in a buffer into rtld from libunwind (guarded by a policy that only exposes it to libunwind), have rtld fill out the data from the trusted stack that we passed in (which would be sealed using the jmpbuf) and then populate the buffer. rtld would remain unaware about libunwind internals, and libunwind could then use that buffer to setCapabilityRegister & co.

dpgao · 2024-06-21T12:08:15Z

Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.

Why would we have to teach it about libunwind internals? The idea would be to pass in a buffer into rtld from libunwind (guarded by a policy that only exposes it to libunwind), have rtld fill out the data from the trusted stack that we passed in (which would be sealed using the jmpbuf) and then populate the buffer. rtld would remain unaware about libunwind internals, and libunwind could then use that buffer to setCapabilityRegister & co.

Oh, I see what you mean now. Yes, delegating the task of restoreRegistersFromSandbox to RTLD is a good idea. And we no longer need to expose the sealer to libunwind, but instead need an API for libunwind to query whether c18n is enabled.

I'll implement that and push a patch to CTSRD-CHERI/cheribsd#2122.

dstolfa

I ran the tests and it works for me. A few nits in the code, but overall I'm fine with this change.

libunwind/src/CompartmentInfo.hpp

jrtc27

Happy to see the cumulative diff significantly reducing and becoming much more additive instead of invasive. Some comments to polish it up.

libunwind/src/CompartmentInfo.hpp

libunwind/src/DwarfInstructions.hpp

libunwind/src/UnwindRegistersSave.S

libunwind/src/DwarfInstructions.hpp

libunwind/src/CompartmentInfo.hpp

libunwind/src/DwarfInstructions.hpp

dpgao · 2024-08-28T17:10:25Z

@jrtc27 I removed the LIBUNWIND_CHERI_C18N_SUPPORT option so that c18n support code is always compiled on purecap Morello. I also moved all the unwinding logic into one function in struct CompartmentInfo which is now called in UnwindCursor.

jrtc27 · 2024-08-29T18:06:57Z

libunwind/src/CompartmentInfo.hpp

@@ -13,21 +13,68 @@
 #ifndef __COMPARTMENT_INFO_HPP__
 #define __COMPARTMENT_INFO_HPP__

+extern "C" {
+
+#include <link.h>


What's this for?

Since the declarations for dl_c18n_is_tramp and dl_c18n_pop_trusted_stk are now in link_elf.h, I include link.h to avoid having to re-declare both functions.

Ok, but then you need to detect them in CMake, because otherwise you'll break the build on Linux or current/old versions of CheriBSD.

Also, why isn't the struct declared by system headers then?..

Hmmm maybe it's easier to just hard-code everything here in libunwind then.

Well, test it? See if you can build against old and new CheriBSD.

And make sure to test against CHERI-RISC-V too.

OK. I've successfully built the latest commit with CTSRD-CHERI/cheribsd#2123 for both Morello and CHERI-RISC-V, and it also builds on an older CheriBSD installation through CMake.

And when built for Morello with 2123 it detected c18n support?

Yes, CMake detects c18n support on 2123.

libunwind/src/CompartmentInfo.hpp

libunwind/src/UnwindCursor.hpp

jrtc27 · 2024-08-29T18:20:55Z

libunwind/src/UnwindRegistersRestore.S

@@ -731,8 +712,12 @@ WEAK_ALIAS(__rtld_unw_setcontext, _rtld_unw_setcontext)
  .p2align 2
 DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto)
 #ifdef __CHERI_PURE_CAPABILITY__
+  ldr    c2,      [c0, #0x1f0]  // Pass the target untrusted stack pointer
+  ldr    c3,      [c0, #0x210]  // Pass the target trusted stack pointer
+  bl     dl_c18n_unwind_trusted_stk


I would much rather not have all this magic "pass through an arbitrary set of arguments as the return values so those registers are preserved, according to whatever the caller this is designed for happens to need" and instead do one of:

Follow the ABI of a normal function call

Use a highly-customised calling convention like TLSDESC does

I don't think any of the callers of these kinds of functions is performance-critical enough to warrant 2.

The latest commit now does 1.

libunwind/src/UnwindCursor.hpp

jrtc27 · 2024-10-18T17:28:54Z

libunwind/src/CompartmentInfo.hpp

@@ -13,21 +13,68 @@
 #ifndef __COMPARTMENT_INFO_HPP__
 #define __COMPARTMENT_INFO_HPP__

+extern "C" {
+
+#include <link.h>


Please do it properly. Detect them in CMake and make unwindIfAtBoundary a stub if not present (with CheriBSD's config indicating their presence). You also won't need stubs for _dl_c18n_get/unwind_trusted_stk then too.

libunwind/include/__libunwind_config.h

libunwind/CMakeLists.txt

jrtc27 · 2024-10-21T16:49:16Z

libunwind/src/UnwindRegistersRestore.S

@@ -731,8 +712,18 @@ WEAK_ALIAS(__rtld_unw_setcontext, _rtld_unw_setcontext)
  .p2align 2
 DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto)
 #ifdef __CHERI_PURE_CAPABILITY__
+#ifdef _LIBUNWIND_HAS_CHERI_LIB_C18N
+  // Preserve the argument in a callee-saved register instead of pushing it onto
+  // the stack because stack unwinding will switch the stack.


Whilst using a saved register is better anyway, this justification seems undesirable, shouldn't it leave us on the same stack? Then when this function / longjmp returns it can restore the real stack that corresponds to what the compartment should have.

We can leave the stack unchanged, but then there would be a small window between after dl_c18n_unwind_trusted_stk returns and the new stack is installed where the trusted stack says we're in the target compartment but the stack pointer still contains that of the source compartment. This doesn't seem desirable.

Under the current implementation, after dl_c18n_unwind_trusted_stk returns, we are for all purposes already in the target compartment (i.e. not even in libunwind's compartment anymore). And the final ret resumes execution of the target compartment without going through a trampoline. (Because unw_getcontext is a trusted function, it has access to the raw return capability to the target compartment.)

jrtc27 · 2024-10-21T16:50:03Z

libunwind/src/UnwindRegistersSave.S

+#ifdef _LIBUNWIND_HAS_CHERI_LIB_C18N
+  // Store the trusted stack pointer
+  stp    c0, c30, [csp, #-0x20]!
+  bl     dl_c18n_get_trusted_stk


What happens when we return back to the calling compartment? Do we not end up referencing a trusted frame that no longer exists?

The implementation of dl_c18n_get_trusted_stk already accounts for this and returns the trusted frame below the top one.

Well that doesn't work if you have, say, something within [TCB] call setjmp?

That seems surprising and fragile, no?

Well, setjmp is marked as a trusted symbol so calling it anywhere won't result in a new frame being pushed. setjmp calling dl_c18n_get_trusted_stk will push a new frame which will be skipped over in the implementation of dl_c18n_get_trusted_stk. So this always works.

unw_getcontext is also marked as trusted for that matter.

Hm, it's not ideal having that as a requirement for the caller. In these contexts it's fine, but ideally it'd be general enough that you could ask for your own trusted frame or your caller's. Anything more than that is going to need the full complexity of unwinding involved to be useful so having to go through libunwind to do it is fine, but I could imagine both those options being useful.

To throw some ideas around, you could:

Have dl_c18n_get_trusted_stack take a bool for whether to pop a frame (you'd have to have the caller also call dl_c18_is_trampoline on CLR unless it knows it's a trusted symbol)

Have dl_c18n_get_trusted_stack take a pointer (i.e. CLR) which it then calls dl_c18n_is_trampoline on and proceeds like the above (with NULL being treated as not a trampoline, i.e. get the current frame, if dl_c18n_is_trampoline itself doesn't do that)

Sure. I've implemented the second option and modified dl_c18n_is_trampoline to check for the tag on the CLR passed in.

jrtc27 · 2024-10-24T18:47:53Z

libunwind/src/UnwindRegistersSave.S

+#ifdef _LIBUNWIND_HAS_CHERI_LIB_C18N
+  // Store the trusted stack pointer
+  stp    c0, c30, [csp, #-0x20]!
+  mov    x0, xzr


Is there a reason not to pass c30 here and let it automatically do the right thing, beyond the overhead of doing the trampoline check?

No, but I can certainly change it to pass c30.

Assembly stubs for _rtld_unw_{get,set}context are no longer needed. Due to the significantly simplified implementation, the LIBUNWIND_CHERI_C18N_SUPPORT option has been removed and c18n support is now included by default for supported architectures (currently Morello only).

jrtc27

This looks ok to my eye now, especially when looking at https://github.com/CTSRD-CHERI/llvm-project/compare/559dbe0..unwind-patch (and ignoring purecap-benchmark changes)

libunwind/src/UnwindRegistersSave.S

dpgao mentioned this pull request Jun 20, 2024

[RFC] c18n: New {get,set}context APIs for libunwind CTSRD-CHERI/cheribsd#2122

Closed

dstolfa approved these changes Jun 20, 2024

View reviewed changes

dpgao force-pushed the unwind-patch branch from 15fe510 to 87f9a3f Compare July 18, 2024 14:45

dpgao requested review from dstolfa and jrtc27 August 22, 2024 19:45

dstolfa reviewed Aug 24, 2024

View reviewed changes

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

libunwind/src/CompartmentInfo.hpp Show resolved Hide resolved

dstolfa mentioned this pull request Aug 24, 2024

c18n: New {get,set}context APIs for libunwind CTSRD-CHERI/cheribsd#2123

Open

dpgao force-pushed the unwind-patch branch from 87f9a3f to faf5ea2 Compare August 27, 2024 14:05

jrtc27 reviewed Aug 27, 2024

View reviewed changes

libunwind/src/DwarfInstructions.hpp Outdated Show resolved Hide resolved

dpgao force-pushed the unwind-patch branch from faf5ea2 to 16e8292 Compare August 28, 2024 16:58

dpgao requested review from jrtc27 and dstolfa August 28, 2024 17:10

jrtc27 reviewed Aug 29, 2024

View reviewed changes

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

jrtc27 reviewed Aug 29, 2024

View reviewed changes

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

jrtc27 reviewed Aug 29, 2024

View reviewed changes

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

jrtc27 reviewed Aug 29, 2024

View reviewed changes

libunwind/src/UnwindCursor.hpp Outdated Show resolved Hide resolved

jrtc27 reviewed Aug 29, 2024

View reviewed changes

dpgao force-pushed the unwind-patch branch from 16e8292 to a0cfcb6 Compare August 29, 2024 20:28

dpgao requested a review from jrtc27 August 29, 2024 20:33

dpgao changed the title ~~libunwind: Simplify _unw_{get,set}context under c18n~~ libunwind: Use new unwinding APIs under c18n Oct 18, 2024

jrtc27 reviewed Oct 18, 2024

View reviewed changes

dpgao force-pushed the unwind-patch branch 2 times, most recently from fb33dfe to e7e5fff Compare October 18, 2024 20:59

jrtc27 reviewed Oct 18, 2024

View reviewed changes

libunwind/include/__libunwind_config.h Outdated Show resolved Hide resolved

jrtc27 reviewed Oct 18, 2024

View reviewed changes

libunwind/CMakeLists.txt Show resolved Hide resolved

dpgao force-pushed the unwind-patch branch from e7e5fff to 4c97ca9 Compare October 19, 2024 14:44

jrtc27 reviewed Oct 21, 2024

View reviewed changes

dpgao force-pushed the unwind-patch branch from 4c97ca9 to c613583 Compare October 22, 2024 10:47

dpgao requested a review from jrtc27 October 23, 2024 16:49

dpgao force-pushed the unwind-patch branch from c613583 to 7f6e1b2 Compare October 24, 2024 13:59

jrtc27 reviewed Oct 24, 2024

View reviewed changes

dpgao force-pushed the unwind-patch branch from 7f6e1b2 to 8112874 Compare October 24, 2024 19:41

jrtc27 approved these changes Oct 25, 2024

View reviewed changes

dpgao commented Oct 25, 2024

View reviewed changes

libunwind/src/UnwindRegistersSave.S Outdated Show resolved Hide resolved

Fix typo in comment

fd6ea94

dpgao merged commit 82d5ca8 into dev Oct 25, 2024
0 of 4 checks passed

dpgao deleted the unwind-patch branch October 25, 2024 18:26

libunwind: Use new unwinding APIs under c18n #740

libunwind: Use new unwinding APIs under c18n #740

Conversation

dpgao commented Jun 20, 2024 • edited Loading

dstolfa commented Jun 20, 2024

jrtc27 commented Jun 20, 2024

jrtc27 commented Jun 20, 2024

dpgao commented Jun 20, 2024

jrtc27 commented Jun 20, 2024

dpgao commented Jun 21, 2024

dstolfa commented Jun 21, 2024

dpgao commented Jun 21, 2024

jrtc27 commented Jun 21, 2024

dstolfa commented Jun 21, 2024

dpgao commented Jun 21, 2024

dstolfa left a comment

Choose a reason for hiding this comment

jrtc27 left a comment

Choose a reason for hiding this comment

dpgao commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 left a comment

Choose a reason for hiding this comment

dpgao commented Jun 20, 2024 •

edited

Loading